Similar to #2796, we cannot pull in a dependency on ring which I think this would do. Unfortunately, that's a hard blocker.

Both of these PRs make me wonder if we just need to rethink our dependency on reqwest; rather, how we depend on it. Considering that resolver version >= 2 take a union of features in the dependency tree, why should we even get into the business of which reqwest feature we take? Obviously we need to optionally depend on gzip and deflate for code we have, but the actual TLS features we don't have a hard dependency on. Maybe we should just avoid any such dependencies entirely; unless, of course, that pulls in ring by default. But considering that resolve v2+ doesn't support mutually exclusive features, we could still take a dependency on native-tls-native-roots-no-provider and people can depend on whatever other features they want in reqwest, right? Would that actually work?

Thank you for having a look at this :)

I am not sure if you meant to comment on this line specifically or whether you commented on the goal of the PR in general. This line here is only renaming a feature of the typespec_client_core crate. There should not be any change in the pulled dependencies because of it. I think you could also check the Cargo.lock file's diff as (IIUIC) it should always include entries for all possible dependencies. For this PR, it is only extended with an entry for webpki-roots.

I changed this feature's name because of a slightly weird (to me at least) cargo behavior: Cargo auto-enables an optional dependency when a feature from our crate is depending on a feature of the dependency (crate/feature syntax). However, it also will activate any feature of our crate with the name of the dependency while doing so.

Concretely, this meant that when I want to build only with the reqwest_rustls_webpki_roots feature (which is the goal of my PR, i.e. not accidentally pulling in openssl as the native certs provider), I would need to enable reqwest/rustls-tls-webpki-roots-no-provider, hence enabling the reqwest dependency AND our reqwest feature, which was then enabling reqwest/native-tls and thus breaking what I set out to do.

I changed this feature's name because of a slightly weird (to me at least) cargo behavior: Cargo auto-enables an optional dependency when a feature from our crate is depending on a feature of the dependency (crate/feature syntax). However, it also will activate any feature of our crate with the name of the dependency while doing so.

I'm not sure I fully understand what you're saying here. You're saying you have a feature named "reqwest" in your crate as well?

What I'm specifying here in Cargo.toml is that if you want to use reqwest, we default to using the native TLS feature so that there's some TLS provided. I don't want to start getting into 1:1 parity with dependencies' feature names when a consumer can just as easily take a dependency on reqwest themselves and enable whatever other features they want, since cargo's resolver v2 and newer takes a union of features and mutually exclusive features aren't supported.

This is why I'm proposing that instead of adding more features (or even renaming), consumers can add a direct dependency to reqwest themselves and pass in their own HttpClient if they want, which we support fairly easily.

We're not going to take this PR this week as we're about to release core and the other crates, but I want to do a little usability test to see if this is feasible. Our decision to provide a default but not required HTTP stack should not also require us on behalf of consumers to start doing 1:1 parity of dependencies' features. That will be difficult to maintain and difficult for consumers to use.

I changed this feature's name because of a slightly weird (to me at least) cargo behavior: Cargo auto-enables an optional dependency when a feature from our crate is depending on a feature of the dependency (crate/feature syntax). However, it also will activate any feature of our crate with the name of the dependency while doing so.

I'm not sure I fully understand what you're saying here. You're saying you have a feature named "reqwest" in your crate as well?

With "our crate" I meant typespec_client_core. I'm only trying to point out the technical issue that when the feature is named reqwest like the dependency, any usage of reqwest/some-feature will enable the reqwest feature (i.e. not only the dependency). And then - when the reqwest feature is defined as ["reqwest/native-tls"] we pull in openssl on linux which is what I want to avoid.

I don't want to start getting into 1:1 parity with dependencies' feature names

I get this and share the feeling a 100%. I was looking a bit if I can find out how others in the rust world deal with this. This gave me the impression that maybe people solve it over composition rather than dependencies? Might be totally off, though. Actually I was questioning if this wasn't already doable and I just hadn't thought of it. But I am worried about a couple of sprinkled calls to new_http_client from sdk/typespec/typespec_client_core/src/http/clients/mod.rs (especially in impl Default for TransportOptions). But maybe this is the direction to pursue, after all - i.e. only conditionally provide default construction of anything that needs an http client if a backend is explicitly enabled via a feature (which it would be by default)

we default to using the native TLS feature so that there's some TLS provided.

I think that it could be better to express this by having reqwest/native_tls in typespec_client_core's default features rather than in the overall reqwest feature, though. That's what I was doing in this PR.

We're not going to take this PR this week as we're about to release core and the other crates

No worries. I am also fine if we do not end up proceeding with the PR at all. Installing openssl into all our images was more a nuisance rather than a blocker for us. Naively I thought it was going to be a minor thing to fix. But I am starting to realize it very much is not 😄 Overall, I am really happy that I got attention from you for this "problem" and grateful for the time you have spent to give me feedback.

tokio-rs/axum#1032 gave me the impression that maybe people solve it over composition rather than dependencies?

We do support composition - we have to, since 1P will not be using tokio for async, for example (which also rules out reqwest built on hyper) - but we still provide a default for the vast majority of consumers who either won't care, or will likely be using tokio/reqwest as the almost de facto async/HTTP stacks. Therein lies the problem of dependencies. You can "disable" reqwest and use your own stack and we shouldn't pull in anything.

I think that it could be better to express this by having reqwest/native_tls in typespec_client_core's default features rather than in the overall reqwest feature, though. That's what I was doing in this PR.

Given the behavior you noted - I had not noticed that myself but it doesn't surprise me - that indeed may be a good rename but, as noted, we won't take it for this imminent release.

Avoiding the unwanted openssl dependency is certainly justified. It's a pain to get on Windows, though I don't suspect many servers running Rust are probably running on Windows/Windows containers. Still, no reason to make acquisition and deployment harder.

We do support composition

Fair - I suppose TransportOptions can be specified in all relevant places with any HttpClient implementations users could come up with. I should try this out! Within the scope of this SDK I'm worried, however, that new_http_client makes it a bit too easy to accidentally create APIs that just assume one can always be materialized from thin air. Am I mistaken here? Apologies for the silly question, but I actually do not know: In which use cases is falling back to NoopClient desired behavior? Would it maybe be feasible to only make new_http_client (and all implementations of some impl Default using it) available if the reqwest feature is active? I.e. without any NoopClient fallback, thus forcing correct HttpClient provision?

It's a pain to get on Windows, though I don't suspect many servers running Rust are probably running on Windows/Windows containers

I found it a bit surprising that openssl would become a problem on windows - the native_tls crate's docs state

This crate uses SChannel on Windows (via the schannel crate), Secure Transport on OSX (via the security-framework crate), and OpenSSL (via the openssl crate) on all other platforms.

So I would assume on windows it should not be part of the build-time dependencies(?).